-
Notifications
You must be signed in to change notification settings - Fork 162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update hippo client #283
base: main
Are you sure you want to change the base?
update hippo client #283
Conversation
ybtsdst
commented
Mar 1, 2024
- update hippo client
- enable hippo client
- add more params
- enable scalar index for filter search cases
- some small improvements
- enable avg latency
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ybtsdst The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
||
[*.json] | ||
indent_style = space | ||
indent_size = 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
each file should end with a newline, same for other files.
@@ -0,0 +1,93 @@ | |||
from transwarp_hippo_api.hippo_client import HippoClient, HippoField |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this file be included in this PR? Seems it's just for debugging purpose, we should keep hippo client same as others.
@@ -161,7 +161,7 @@ def _load_train_data(self): | |||
finally: | |||
runner = None | |||
|
|||
def _serial_search(self) -> tuple[float, float]: | |||
def _serial_search(self) -> tuple[float, float, float]: | |||
"""Performance serial tests, search the entire test data once, | |||
calculate the recall, serial_latency_p99 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also change the docstring? TBH I think the avg latency should be in a seperate PR ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ybtsdst Thanks for contribution!
Hippo clients part LGTM, but this PR carries many unnececcery changes as @zhjwpku suggests. I'll merge this PR if this PR contians changes of hippo client only. Which means the follow change requests:
- remove all the changes about editor and vscode, except
.gitignore
- remove the
test.py
in dirhippo/
- remove the new added
avg_latency
metric, better in a different PR